Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include median #2049

Closed
wants to merge 6 commits into from
Closed

Include median #2049

wants to merge 6 commits into from

Conversation

lrubiorod
Copy link
Contributor

@lrubiorod lrubiorod commented Aug 30, 2021

Close #2047 and #2054

rad/src/reducers/median.rs Outdated Show resolved Hide resolved
]);
// RoundToInteger means that when the average is not an integer, it will be rounded to an
// integer. For example, the average of 1 and 2, which is 1.5, will be rounded to 1.
mean(&rl, MeanReturnPolicy::RoundToInteger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation of MeanReturnPolicy, RoundToInteger actually means "keep same type as the input":

/// Different available policies regarding what to do with the resulting Float after applying the
/// average mean.
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum MeanReturnPolicy {
    /// Pure `Fn(Array<T>) -> Float` behavior.
    ReturnFloat,
    /// Enforces `Fn(Array<T>) -> T` behavior (if needed).
    RoundToInteger,
}

rad/src/reducers/median.rs Outdated Show resolved Hide resolved
Comment on lines 2180 to 2184
&& !self
.chain_state
.tapi_engine
.wip_activation
.contains_key("WIP0017")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is new: it means that miners will stop signaling when the WIP is activated. Not sure if this is the desired behavior, because this field is also used as a census to see the percentage of miners that are on the latest version. Also, bit 0 is set to 0 in this PR while currently in mainnet it is set to 1. That doesn't break anything consensus-wise but it may be unexpected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this bit only should be used to signaling, and after achieve the necessary votes, it is not longer required. We should remove the activation of this bit, to avoid overlap bit problems in the future

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed opinions. WIP-0014 does not define what to do because it is consensus neutral. It is true that setting bit position 0 to value 0 at this point may come as a surprise, but also we know for certain that we'll need to do so at some point in the coming years. So maybe it is time to define whether we are OK with setting them to 0 upon next bit position being assigned, or we make a commitment to set all bits to 0 when we assign bit 31 (although that removes the ability of signaling 30 and 31 at the same time).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think being able to query which (ARS) nodes have signalled support for a certain WIP after the WIP is passed or defeated is quite a useful feature. Yes, in the (distant) future, bits will be reused and at that point this'll become a problem, but maybe that is not something that should be solved right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an option to leave as before and open an issue with a discussion about that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's just leave it like it was before.

RadonReducers::AverageMedian => match &context.active_wips {
Some(active_wips) if active_wips.wip0017() => median::median(input),
_ => error(),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here if context.active_wips is None, the WIP is assumed to be not activated. This is the default for external tools that use the radon library, because they may not have access to a witnet node so they don't know the active wips. However we need to ensure that the None case means "enabled" after this WIP is activated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about changing the None case to mean enabled even if the WIP is not activated yet? This way external tools can create and run requests that use the new operators, but if they deploy the request before the WIP is active they will get an UnsupportedReducer error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if they create a request, they try it with this feature and obtain a proper result, but when send to mainnet, it fails... It wasnt a good test to try before...

@lrubiorod lrubiorod marked this pull request as ready for review August 31, 2021 15:59
fn test_operate_reduce_median_empty() {
let input = RadonArray::from(vec![]);
let output = median(&input).unwrap_err();
let expected_error = ModeEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: we use ModeEmpty as the error for when the median reducer is applied over an empty array. Should we create a new error MedianEmpty, or rename ModeEmpty to a more generic EmptyArray error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EmptyArray sounds better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take this into account for the eventual WIP, sirs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like this error cannot be easily renamed because it appears in the UnhandledIntercept error message, so changing its name would make some tallies invalid. However we could maybe avoid the UnhandledIntercept by implementing the conversion from RadError to RadonError and assigning an error code to this error. The currently assigned error codes can be found here in the bridge and here in the node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add a TODO or FIXME and create a new issue, because I think it is out of the scope of this issue. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use EmptyArray only in the median by the moment

rad/src/reducers/median.rs Outdated Show resolved Hide resolved
@lrubiorod lrubiorod force-pushed the include_median branch 2 times, most recently from f620bc2 to 285426c Compare September 1, 2021 16:26
oppose_wip0014,
oppose_wip0016,
} = &self.tapi;
let Tapi { oppose_wip0017 } = &self.tapi;

let mut v = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be v = 1, to keep bit 0 enabled?

@tmpolaczyk
Copy link
Contributor

Let's merge this pull request to a new branch, since we don't know the final activation date yet and the WIP is not final yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MEDIAN reducer implementation
5 participants